Param edit table tests#1019
Merged
Merged
Conversation
amilcarlucas
force-pushed
the
param_edit_table_tests
branch
from
7f327a9 to
d09ceca
Compare
amilcarlucas
force-pushed
the
param_edit_table_tests
branch
from
d09ceca to
801efa1
Compare
Contributor
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Contributor
Test Results 3 files ±0 3 suites ±0 2m 52s ⏱️ +3s Results for commit 801efa1. ± Comparison against base commit f9fed24. This pull request removes 18 and adds 19 tests. Note that renamed tests count towards both. |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors parameter value update error handling by introducing a centralized _handle_parameter_value_update method to consolidate duplicate error handling logic across multiple widget interaction points (entry fields, comboboxes, and bitmask editors). The centralization improves code maintainability and consistency in user-facing error messages.
- Adds
_handle_parameter_value_update()method to centralize parameter validation and error handling - Replaces duplicated try-except blocks in entry change handlers, combobox selection handlers, and bitmask editor with calls to the centralized method
- Expands test coverage with new tests for parameter value update handling, UI error/info display, and widget creation edge cases
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/test_frontend_tkinter_parameter_editor_table.py |
Adds comprehensive tests for centralized error handling, including success, unchanged values, out-of-range scenarios, and type errors; adds tests for UI error/info messaging and widget styling |
tests/gui_frontend_tkinter_parameter_editor_table.py |
Adds helper function for creating mock parameters and new user workflow tests for multi-parameter editing, GUI complexity switching, and validation error recovery |
ardupilot_methodic_configurator/frontend_tkinter_parameter_editor_table.py |
Implements centralized _handle_parameter_value_update() method and refactors three call sites to use it instead of duplicated error handling |
Comments suppressed due to low confidence (2)
tests/gui_frontend_tkinter_parameter_editor_table.py:1
- Use of
Unionfrom typing module is outdated for Python 3.10+. The modern syntax uses|operator (which is already used on lines 37, 39-41, 44-45). Either remove this import and update line 38 to usedict | None, or consistently useUnionthroughout. The codebase appears to prefer the|syntax based on the actual usage.
#!/usr/bin/env python3
tests/gui_frontend_tkinter_parameter_editor_table.py:1
- This helper function is nearly identical to the one in
test_frontend_tkinter_parameter_editor_table.py(lines 38-82), creating code duplication. The two functions have different parameter signatures and logic (this one has min/max_value parameters, the other has is_bitmask/is_multiple_choice/is_derived). Consider consolidating into a shared test utility module to reduce duplication and ensure consistency across test files.
#!/usr/bin/env python3
| from ardupilot_methodic_configurator.backend_filesystem import LocalFilesystem | ||
| from ardupilot_methodic_configurator.configuration_manager import ConfigurationManager, InvalidParameterNameError | ||
| from ardupilot_methodic_configurator.data_model_ardupilot_parameter import ArduPilotParameter | ||
| from ardupilot_methodic_configurator.data_model_ardupilot_parameter import ArduPilotParameter, ParameterUnchangedError |
There was a problem hiding this comment.
Missing import for ParameterOutOfRangeError which is used by the _handle_parameter_value_update method being tested. The new tests verify behavior for out-of-range parameters (lines 739-789), but the exception class is not imported. This will cause test failures when the exception is raised.
Suggested change
| from ardupilot_methodic_configurator.data_model_ardupilot_parameter import ArduPilotParameter, ParameterUnchangedError | |
| from ardupilot_methodic_configurator.data_model_ardupilot_parameter import ArduPilotParameter, ParameterUnchangedError, ParameterOutOfRangeError |
| tooltip_call_args = mock_tooltip.call_args[0] | ||
| assert "Add a parameter to the test_file.param file" in tooltip_call_args[1] | ||
|
|
||
| def test_create_flightcontroller_value_sets_correct_background_colors(self, parameter_editor_table) -> None: # pylint: disable=too-many-statements # noqa: PLR0915 |
There was a problem hiding this comment.
[nitpick] This test has 90+ lines and requires complexity suppression comments (too-many-statements, PLR0915). Consider breaking it into separate test methods for each background color scenario (default value, below limit, above limit, unknown bits, no FC value, normal value) to improve readability and maintainability.
| # Assert: Correct widget was returned | ||
| assert result == mock_instance | ||
|
|
||
| def test_create_new_value_entry_shows_error_for_non_editable_parameters(self, parameter_editor_table) -> None: |
There was a problem hiding this comment.
[nitpick] This test validates both forced and derived parameters in a single 95-line test method. Consider splitting into two separate test methods: test_create_new_value_entry_shows_error_for_forced_parameters and test_create_new_value_entry_shows_error_for_derived_parameters to improve clarity and make test failures easier to diagnose.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.